Skip to content

fix(resource): enforce agent-level watch task isolation#762

Merged
myysy merged 3 commits intovolcengine:mainfrom
lyfmt:fix/watch-task-agent-isolation
Mar 19, 2026
Merged

fix(resource): enforce agent-level watch task isolation#762
myysy merged 3 commits intovolcengine:mainfrom
lyfmt:fix/watch-task-agent-isolation

Conversation

@lyfmt
Copy link
Contributor

@lyfmt lyfmt commented Mar 19, 2026

Description

Fixes missing agent-level isolation for watch task permissions.

Previously, watch task access for USER role was effectively scoped by account_id + user_id, which allowed one agent to read, update, or delete watch tasks created by another agent under the same user.

This PR tightens the permission check to account_id + user_id + agent_id for USER role while preserving the existing behavior for ADMIN and ROOT.

Related Issue

N/A

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

  • Updated WatchManager permission checks so USER access now requires both matching user_id and agent_id
  • Propagated ctx.user.agent_id through ResourceService and scheduler paths when querying, updating, and deleting watch tasks
  • Added regression tests covering cross-agent visibility, update/delete denial, admin cross-agent management, and service-layer conflict/cancel behavior

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Test commands:

  • .venv/bin/python -m pytest tests/resource/test_watch_manager.py tests/service/test_resource_service_watch.py -q

Additional validation:

  • Verified with a real local agent/provider setup that:
    • agent A can see and cancel its own watch task
    • agent B cannot read or cancel agent A's watch task
    • agent B still receives URI conflict when attempting to create a duplicate watch on the same target

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

N/A

Additional Notes

  • This change intentionally keeps ADMIN behavior unchanged: admins can still manage watch tasks across agents within the same account.
  • The goal of this PR is to align watch task ownership with the repository's existing user + agent isolation model for agent-scoped state.

@lyfmt lyfmt marked this pull request as ready for review March 19, 2026 05:40
Copy link
Contributor

@qin-ptr qin-ptr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

经过详细分析,这个 PR 正确地修复了 watch task 权限检查中缺少 agent 级别隔离的安全问题。

核心变更

  • 将 USER 角色的权限检查从 account_id + user_id 增强为 account_id + user_id + agent_id
  • 所有调用权限检查的方法都增加了 agent_id 参数
  • ResourceServiceWatchScheduler 的调用链中正确传递了 ctx.user.agent_id
  • ADMIN 和 ROOT 角色的行为保持不变

设计评估

问题真实性:确认问题存在,之前的代码允许同一用户下的不同 agent 访问彼此的 watch tasks

方案合理性:修改在正确的层级(_check_permission 是权限检查的核心方法),直接针对根因

实现完整性:所有调用路径都正确传递 agent_id,包括查询、更新、删除操作

测试覆盖:新增 156 行测试代码,覆盖跨 agent 访问、权限拒绝、ADMIN 管理、URI 冲突检测等关键场景

设计决策

PR 采用了 "agent 间隔离 + URI 全局冲突检测" 的设计:

  • 不同 agent 不能看到彼此的 tasks(隔离性)
  • 但仍然防止不同 agent 创建指向相同 URI 的 tasks(避免重复监控)
  • 测试 test_conflict_when_task_exists_but_hidden_by_other_agent 验证了这个行为是正确的

亮点

  1. 测试覆盖全面,包括单元测试和集成测试
  2. 错误消息更新为 {account_id}/{user_id}/{agent_id},提供更清晰的调试信息
  3. Commit 组织清晰:核心修复 + style 改进分离
  4. 考虑了参数默认值,减少潜在的破坏影响

建议 (non-blocking)

  1. 可以考虑添加测试验证 ROOT 角色可以跨 agent 访问(虽然代码逻辑简单,风险较低)
  2. 可以在 docstring 中更明确地说明设计决策

没有发现任何 blocking issues,代码正确,设计合理。

🤖 I am a bot owned by @qin-ctx.

@myysy myysy self-assigned this Mar 19, 2026
Copy link
Collaborator

@myysy myysy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the comprehensive feature support.

@myysy myysy merged commit 4c920a6 into volcengine:main Mar 19, 2026
7 checks passed
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Mar 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants